-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactors /dois
faceting and adds support for on-demand facets
#1299
Conversation
app/models/doi.rb
Outdated
_key: "desc", | ||
}, | ||
min_doc_count: 1, | ||
def self.all_doi_aggregations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point it could be a constant.
AGGREGATION_DEFINITIONS = {...
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jrhoads Do you have thoughts on where the constant should be defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be a constant within the model.
app/models/doi.rb
Outdated
def self.query_aggregations(disable_facets: false, facets: nil) | ||
return {} if disable_facets | ||
|
||
requested_facets = facets.respond_to?(:split) ? facets.split(",").map(&:strip).map(&:underscore).map(&:to_sym).uniq : default_doi_query_facets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assumes that facets is only allowed to be a string. It could be useful to allow arrays of symbols to be passed in. And treat the string as a special case.
app/models/doi.rb
Outdated
return {} if disable_facets | ||
|
||
requested_facets = facets.respond_to?(:split) ? facets.split(",").map(&:strip).map(&:underscore).map(&:to_sym).uniq : default_doi_query_facets | ||
requested_facets.index_with { |facet| all_doi_aggregations.dig(facet) }.compact |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is similar to the slice method, available on hashes.
so this would become
all_doi_aggregations.slice(*requested_facets)
or
AGGREGATION_DEFINITIONS .slice(*requested_facets)
if using the constant
keys that are not in the original hash are discarded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great work. Some time in the future we may want to refactor this out in to some sort of DoiAggregationSerializer.
…_aggregations to be callable with an array of symbols
Purpose
Refactors approach to OpenSearch aggregations and serialized faceting at
/dois
to reduce calls to OpenSearch for unused facets. Implements newfacets=
URL param that allows the requestor to select from the possible facets.closes: https://github.com/datacite/product-backlog/issues/127 #1301
Approach
Builds the aggregations based on the requested facets, default or otherwise, before assembling the OpenSearch query. Accepts comma-separated values via a new
facets=
URL param that selects from the possible facets. Facets are serialized based on supported and available aggregations in the OpenSearch response.Open Questions and Pre-Merge TODOs
Learning
Types of changes
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Reviewer, please remember our guidelines: